dev/drupal#54 Remove hook_user_login, fixes the masquerade module #31

mlutfy · 2019-09-18T21:02:15Z

The hook_user_login implementation was added by Torrance during the initial port to Drupal8. The Drupal7 implementation does not implement this hook.

It causes problems with the Drupal 'masquerade' module, causing the user to be logged out when attempting to masquerade. This is because the synchronizeUser() function resets the PHP session.

More details here:
https://lab.civicrm.org/dev/drupal/issues/54#note_23652

I tested the following use-cases:

As a Drupal admin, I created a new Drupal user, then logged-in with that user. The contact+user were in sync.
As a CRM admin, with the Actions -> Create CMS account, I created a Drupal account for a user, then logged-in with that account. Also OK.

Looking on the initial commit (29b7571), there doesn't seem to be a reason for the hook, so it feels safe to remove.

mlutfy · 2019-09-19T17:06:33Z

civicrm.user.inc

+    // Also, there is a isMasquerading() function, but it does not seem to be
+    // initialized at this point (it always returns false).
+    $session = \CRM_Core_Session::singleton();
+    $session->reset(FALSE);


On the first (normal) login, the CiviCRM session stores the userID and ufID, but when the user masquerades and synchronizeUser gets called again, the old session ufID doesn't match anymore and so it does a hard reset of the session (causing a logout).

Using $session->reset(FALSE) does a soft reset of the CiviCRM-specific session bits, so that the call to synchronize then re-updates the ufID and whatever is required.

Finally, I tried to reduce the scope of this fix, in case it causes regressions (my first attempt had broken the dashlets). I would have use \Drupal::service('masquerade')->isMasquerading(), but for some reason it did not work and I could not figure out why.

mlutfy · 2019-09-19T17:07:18Z

civicrm.user.inc

@@ -11,6 +11,18 @@ use Drupal\Core\Session\AccountInterface;
 * Implements hook_user_login().
 */
 function civicrm_user_login(AccountInterface $account) {
+  $civicrm = \Drupal::service('civicrm');
+  $civicrm->initialize();


This might not be necessary, but the other functions do it, so I figured it was safer. I didn't want to create a new wrapper function in src/Civicrm.php just for getSession or equivalent (as was done for the synchronize function)

monishdeb · 2019-09-19T17:08:34Z

civicrm.user.inc

+    $session = \CRM_Core_Session::singleton();
+    $session->reset(FALSE);
+  }
+
  \Drupal::service('civicrm')->synchronizeUser($account);
 }


just to reduce LOC

function civicrm_user_login(AccountInterface $account) { $civicrm = \Drupal::service('civicrm'); $civicrm->initialize(); if (\Drupal::hasService('masquerade')) { // In theory this is harmless whether we masquerade or not, but for now narrowing // the scope, in case it causes regressions. // Also, there is a isMasquerading() function, but it does not seem to be // initialized at this point (it always returns false). \CRM_Core_Session::singleton()->reset(FALSE); } $civicrm->synchronizeUser($account); }

mlutfy · 2019-09-23T14:36:52Z

I did another force-push to address another odd behaviour encountered with the Dashboard dashlets.

stesi561 · 2019-09-24T02:36:27Z

Is there some reason that the behaviour: " logged out when attempting to masquerade" would not be observed? I've just being testing this on a pretty vanilla d8 site 8.7.6 civicrm 5.16.2, masquerade 8.x-2.0-beta2 with and without this PR - I can masquerade through to another user - but am unable to unmasquerade (clicking the link results in a log out). Am I missing something basic here?

mlutfy · 2019-09-24T12:10:37Z

@stesi561 Maybe relevant: in most of my test-cases, the Drupal frontpage is the CiviCRM dashboard (/civicrm/dashboard).

Just to double-check: when testing, were you accessing CiviCRM before & after masquerading? I suppose the bug might not trigger if you masquerade immediately after the initial login (since the CiviCRM session will not have been initialized yet).

stesi561 · 2019-09-24T22:19:16Z

Hmm. Yes okay - I can replicate the problem behaviour consistently now.

However applying the above patch doesn't seem to resolve?

Steps to reproduce:
Apply the patch,
clear caches wipe templates_c,
log in
Navigate to /civicrm
Masquerade as another user
This seems to succeed
Then navigate to another page - you are now logged out.

I've upgraded the site I was using to test on to civicrm 5.17.4, and drupal 8.7.7 and tested again.

This is on a drupal site installed using roundearth composer

stesi561 · 2019-09-24T22:21:57Z

Any suggestions on further debugging that I can do to investigate this further?

jitendrapurohit · 2019-10-25T08:51:20Z

civicrm.user.inc

+    $ufID = $session->get('ufID');
+
+    $userSystem = CRM_Core_Config::singleton()->userSystem;
+    $userSystemID = $userSystem->getBestUFID($user);


looks like $user variable is not defined in this function?

👍 yikes, indeed. Although it produced a notice, but it was still giving the correct behaviour because getBestUFID accepts null.

I fixed the code nonetheless and tested to make sure that fixing it did not introduce a regression.

mlutfy · 2020-01-29T02:38:50Z

I tried re-testing this issue after: civicrm/civicrm-core#16177 - thinking it would fix the issue, but I'm still encountering it.

GValFr35 · 2020-01-29T09:31:20Z

I tested civicrm/civicrm-core#16177 in combination with this change :

function civicrm_user_login(AccountInterface $account) {
  $civicrm = \Drupal::service('civicrm');
  $civicrm->initialize();

  if (\Drupal::hasService('masquerade')) {
    // In theory this is harmless whether we masquerade or not, but for now narrowing
    // the scope, in case it causes regressions.
    // Also, there is a isMasquerading() function, but it does not seem to be
    // initialized at this point (it always returns false).
    \CRM_Core_Session::singleton()->reset(FALSE);
  }
  $civicrm->synchronizeUser($account);
}

and that seems to fix the issue on my side without regression (ie dashboard ...).

GValFr35 · 2020-01-29T09:47:50Z

Although I prefer to patch the CRM_Core_Session::reset() function as follows:

  public function reset($all = 1) {
    $this->initialize();
    // to make certain we clear it, first initialize it to empty
    $this->_session[$this->_key] = [];
    unset($this->_session[$this->_key]);

    if ($all) {
      unset($this->_session);
    }
  }

But yes, it must be fully tested with other CMS...

mlutfy · 2021-01-07T16:07:34Z

@GValFr35 Thanks for your input. I updated my PR using your code. Cleaner and works well.

Since this has been open for a while, and I'm not the only one running into this problem, I would be tempted to merge if @GValFr35 validates the latest edit.

(I only patches in the civicrm_user_login, because I would very much prefer to stay away from doing changes to the core session code.)

albionbrown · 2021-07-20T14:12:25Z

I can also say that @GValFr35's patch also works for me too, using Drupal 8.9.x and CiviCRM 5.39.0. Is there any news on when this could get merged?

demeritcowboy · 2021-07-20T14:52:01Z

Since two people have confirmed recently it works for masquerade, I think I'm ok to merge this since the first thing synchronizeUser does is call initialize, so there should be no change if masquerade is not installed. And just doing a quick test like that nothing obvious comes up.

jptillman · 2021-07-20T15:23:27Z

I've been using the last patch via composer in production since it was posted. We rely on it.

mlutfy force-pushed the remove-user-login-hook branch from cfb30dd to 67f628e Compare September 19, 2019 17:00

mlutfy commented Sep 19, 2019

View reviewed changes

monishdeb reviewed Sep 19, 2019

View reviewed changes

mlutfy force-pushed the remove-user-login-hook branch 3 times, most recently from c9bf234 to d6c474f Compare September 23, 2019 14:36

jitendrapurohit reviewed Oct 25, 2019

View reviewed changes

mlutfy force-pushed the remove-user-login-hook branch from d6c474f to 887b28f Compare November 14, 2019 16:38

dev/drupal#54 hook_user_login: workaround for the masquerade module

a6a5014

mlutfy force-pushed the remove-user-login-hook branch from 887b28f to a6a5014 Compare January 7, 2021 16:04

demeritcowboy merged commit 1b8d09a into civicrm:master Jul 20, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dev/drupal#54 Remove hook_user_login, fixes the masquerade module #31

dev/drupal#54 Remove hook_user_login, fixes the masquerade module #31

mlutfy commented Sep 18, 2019

mlutfy Sep 19, 2019

mlutfy Sep 19, 2019

monishdeb Sep 19, 2019 •

edited

Loading

mlutfy Sep 19, 2019

monishdeb Sep 19, 2019

hansrossel Jul 20, 2021

mlutfy commented Sep 23, 2019

stesi561 commented Sep 24, 2019

mlutfy commented Sep 24, 2019

stesi561 commented Sep 24, 2019

stesi561 commented Sep 24, 2019

jitendrapurohit Oct 25, 2019

mlutfy Nov 14, 2019

mlutfy commented Jan 29, 2020

GValFr35 commented Jan 29, 2020 •

edited

Loading

GValFr35 commented Jan 29, 2020

mlutfy commented Jan 7, 2021

albionbrown commented Jul 20, 2021

demeritcowboy commented Jul 20, 2021

jptillman commented Jul 20, 2021

dev/drupal#54 Remove hook_user_login, fixes the masquerade module #31

dev/drupal#54 Remove hook_user_login, fixes the masquerade module #31

Conversation

mlutfy commented Sep 18, 2019

mlutfy Sep 19, 2019

Choose a reason for hiding this comment

mlutfy Sep 19, 2019

Choose a reason for hiding this comment

monishdeb Sep 19, 2019 • edited Loading

Choose a reason for hiding this comment

mlutfy Sep 19, 2019

Choose a reason for hiding this comment

monishdeb Sep 19, 2019

Choose a reason for hiding this comment

hansrossel Jul 20, 2021

Choose a reason for hiding this comment

mlutfy commented Sep 23, 2019

stesi561 commented Sep 24, 2019

mlutfy commented Sep 24, 2019

stesi561 commented Sep 24, 2019

stesi561 commented Sep 24, 2019

jitendrapurohit Oct 25, 2019

Choose a reason for hiding this comment

mlutfy Nov 14, 2019

Choose a reason for hiding this comment

mlutfy commented Jan 29, 2020

GValFr35 commented Jan 29, 2020 • edited Loading

GValFr35 commented Jan 29, 2020

mlutfy commented Jan 7, 2021

albionbrown commented Jul 20, 2021

demeritcowboy commented Jul 20, 2021

jptillman commented Jul 20, 2021

monishdeb Sep 19, 2019 •

edited

Loading

GValFr35 commented Jan 29, 2020 •

edited

Loading